Refactor internal property descriptors to not be ScriptableObjects.
#2143
Conversation
aardvark179
changed the title
ScriptableObjects.
aardvark179
force-pushed
the
aardvark179-property-operations-fix
branch
from
1245ba7 to
6fe3f0a
Compare
| } | ||
|
|
||
| public static final class DescriptorInfo { | ||
| Object enumerable; |
There was a problem hiding this comment.
Wouldn't this code be cleaner overall if DescriptorInfo had accessors that validated the whole NOT_FOUND bit (isEnumerable, isConfigurable, etc)?
| Object getter, | ||
| Object setter, | ||
| Object value) { | ||
| this.enumerable = enumerable; |
There was a problem hiding this comment.
I know it makes the code bigger but would you consider a "builder" pattern for this? I worry about being able to read the code when every DescriptorInfo constructor has six parameters of the same type in a row.
| } | ||
| } | ||
|
|
||
| protected void checkPropertyDefinition(DescriptorInfo desc) { |
There was a problem hiding this comment.
Should this be a member of the DescriptorInfo class?
| return hasProperty(desc, "get") || hasProperty(desc, "set"); | ||
| } | ||
|
|
||
| protected static boolean isAccessorDescriptor(DescriptorInfo desc) { |
There was a problem hiding this comment.
Should this be a member of the DescriptorInfo class?
| return !isDataDescriptor(desc) && !isAccessorDescriptor(desc); | ||
| } | ||
|
|
||
| protected static boolean isGenericDescriptor(DescriptorInfo desc) { |
There was a problem hiding this comment.
Member of the class -- it's a static function that takes one argument...
| return false; | ||
| } | ||
| if (Boolean.FALSE.equals(getProperty(desc, "enumerable"))) { | ||
| if (Boolean.FALSE.equals(desc.enumerable)) { |
There was a problem hiding this comment.
Would this code (and many places like it) be easier to read and follow if it could call "!descriptor.isEnumerable()" or if NOT_FOUND is a special third case, "descriptor.isNotEnumerable()"
I agree. I held off doing that initially because an extremely mechanical change was slightly easier to produce, but it is definitely more readable with a better API. After a little experimenting I'm erring towards something like: boolean isEnumerable() // check for a simple value of true
boolean isEnumerable(boolean value) // check for an explicitly set value that matches
boolean hasEnumerable() // check that a value has been setThe reason for that is that there are a lot of cases where we care that an explicit value of desc.hasEnumerable() && !desc.isEnumerable()is asking for bugs in the operator precedence. |
|
That makes sense -- I had been thinking that we'd need "isEnumerable" and "isNotEnumerable" but I think that your way is more Java-like! |
aardvark179
force-pushed
the
aardvark179-property-operations-fix
branch
from
6fe3f0a to
9a7546f
Compare
|
Thanks -- looks good! |
2 participants
More preparation for separating the concepts of scopes and scriptable objects.
Property descriptors as described in https://tc39.es/ecma262/#sec-property-descriptor-specification-type are not JavaScript objects, but merely internal Record objects with a particular set of fields. Meanwhile FromPropertyDescriptor creates a corresponding JavaScript object and sets a prototype based on the current realm.
While prototyping separating scopes and realms, and reducing where we relied on parent scopes it became clear that our conflation of these two objects made it impossible to get some aspects of the spec correct without altering many of our APIs to pass a scope through. We had already introduced a
DescriptorInfoobject to avoid potential deadlocks with slot map operations, so this PR expands its use to pretty much everywhere where the specs specifies a property descriptor.